feat: Readable metrics view url params #5675
Conversation
AdityaHegde
force-pushed
the
adityahegde/human-readable-url
branch
from
fd6f6c1
to
af1a029
Compare
There was a problem hiding this comment.
Will we be backwards compatible with existing reports?
| convertMetricsEntityToURLSearchParams( | ||
| dashboard, | ||
| url.searchParams, | ||
| exploreSpec ?? {}, | ||
| exploreSpec?.defaultPreset ?? {}, | ||
| ); |
There was a problem hiding this comment.
It's confusing to understand what this function does. A pure function could be more clear e.g.:
const searchParams = getURLSearchParamsFromMetricsEntity(...)
| $: ({ resourceName } = data.token); | ||
| $: ({ | ||
| basePreset, | ||
| partialMetrics, |
There was a problem hiding this comment.
Can we call this partialExploreState?
| toTimeRangesUrl(metrics, searchParams, preset); | ||
|
|
||
| toOverviewUrl(metrics, searchParams, explore, preset); | ||
|
|
||
| toTimeDimensionUrlParams(metrics, searchParams, preset); | ||
|
|
||
| toPivotUrlParams(metrics, searchParams, preset); |
There was a problem hiding this comment.
IMO pure functions would be easier to understand
| } from "@rilldata/web-common/runtime-client"; | ||
|
|
||
| export function convertMetricsExploreToPreset( | ||
| metrics: Partial<MetricsExplorerEntity>, |
There was a problem hiding this comment.
It's confusing to see this called "metrics". Can we move toward using exploreState?
| explore, | ||
| AD_BIDS_TIME_RANGE_SUMMARY, | ||
| ); | ||
| cleanMetricsExplore(initEntity); |
There was a problem hiding this comment.
Why is this necessary?
| // cleans up any UI only state from MetricsExplorerEntity | ||
| export function cleanMetricsExplore( |
There was a problem hiding this comment.
This comment implies the function name could be removeUIStateFromMetricsExplorerEntity. But doesn't the MetricsExplorerEntity contain all UI/client-side state?
| return { | ||
| timeZone: getLocalIANA(), | ||
| }; |
There was a problem hiding this comment.
I wouldn't expect this default/fallback to be included in this function. I'd expect timeZone: getLocalIANA() to be included in the default generic Explore state, which would later be merged with user preferences.
| return { | ||
| explore, | ||
| metricsView, | ||
| basePreset, |
There was a problem hiding this comment.
What about initialExploreState?
| @@ -16,20 +15,21 @@ | |||
| const queryClient = useQueryClient(); | |||
|
|
|||
| export let data: PageData; | |||
| $: ({ metricsView, explore, basePreset, partialMetrics } = data); | |||
There was a problem hiding this comment.
It's confusing seeing both basePreset and partialMetrics. Could we merge the two of them further upstream?
| const basePreset = getBasePreset( | ||
| exploreResource.explore.state?.validSpec ?? {}, | ||
| getLocalUserPreferencesState(exploreName), | ||
| ); |
There was a problem hiding this comment.
It feels like this doesn't belong in this function. Maybe it could be pushed down to a central place where we synthesize the initial state.
No description provided.